-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Fix traceId discrepancy in case error in servlet web #17134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 6.4.x
Are you sure you want to change the base?
Conversation
cc7ac16
to
f526143
Compare
PTAL @jzheaux |
… version until spring-projects/spring-security#17134 will be merged
@jzheaux |
@marcusdacoregio @jzheaux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and the sample application, @nkonev. I've left some feedback inline.
Also, I think we should try adding a test for it. That may mean dispatching to ERROR
manually in the test.
web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
5964208
to
33594ec
Compare
Thank you for review @jzheaux ! UPD: I managed to make the test in |
50483e0
to
4790f73
Compare
Signed-off-by: Nikita Konev <nikit.cpp@yandex.ru>
Fixes #12610
As I see from my research, the bug happens only in the real servlet environment, supposedly because of HttpServletResponse.sendError(), I didn't manage to reproduce it via mockMvc.
Tomcat creates a new HttpServletRequest in case error, which is related to "/error" path, which will be served by ErrorController.
Fortunately, request attributes are copied from the original request to the "/error"-related one, so we can put the ObservationView into the original request in order to extract this ObservationView from "/error"-related request and create a new parent observation with a given parent ObservationView. The last ObservationView propagates TraceContext to the new Observation so we have the same traceId.
Reproducer is here https://github.com/nkonev/trace-discrepancy-reproducer
To check that the bug is fixed, you can run
publishMavenJavaPublicationToMavenLocal
in spring-security:webor via IDE

then switch to the SNAPSHOT dependency nkonev/trace-discrepancy-reproducer@131a271
Before:


After:


UPD:
I also checked this PR with jetty and undertow, it works fine.